synchronize: quotes around arguments #213
Conversation
|
cc @quidame @saito-hideki @aminvakil Could you please review this? Thanks in advance. |
|
|
||
| if rsync_path: | ||
| cmd.append('--rsync-path=%s' % rsync_path) |
There was a problem hiding this comment.
Would it be possible to add tests for this change?
There was a problem hiding this comment.
There is already a test for it.
https://github.com/ansible-collections/ansible.posix/blob/main/tests/integration/targets/synchronize/tasks/main.yml#L154-L166
As @quidame mentioned later the integration tests were disabled sometime ago. I now enabled them. localy with docker it worked
TASK [synchronize : synchronize files using rsync_path (issue#7182)] ***********
changed: [testhost] => {"changed": true, "cmd": "/usr/bin/rsync --delay-updates -F --compress --archive '--rsync-path=sudo rsync' '--out-format=<<CHANGED>>%i %n%L' /root/ansible_collections/ansible/posix/tests/output/.tmp/output_dir/foo.txt /root/ansible_collections/ansible/posix/tests/output/.tmp/output_dir/foo.rsync_path", "msg": ">f+++++++++ foo.txt\n", "rc": 0, "stdout_lines": [">f+++++++++ foo.txt"]}
TASK [synchronize : assert] ****************************************************
ok: [testhost] => {
"changed": false,
"msg": "All assertions passed"
}
TASK [synchronize : Cleanup] ***************************************************
changed: [testhost] => (item=foo.rsync_path) => {"ansible_loop_var": "item", "changed": true, "item": "foo.rsync_path", "path": "/root/ansible_collections/ansible/posix/tests/output/.tmp/output_dir/foo.rsync_path", "state": "absent"}
|
hi @flybyray , thanks for fixing this ! |
/pull/213#issuecomment-876480707
There was a problem hiding this comment.
Based on the code changes, I think we probably need to append the integration test like below:
- name: command-line options are quoted correctly (issue#24 and issue#190)
synchronize:
src: "{{output_dir}}/foo.txt"
dest: "dummy:{{output_dir}}/foo.rsync_path_1"
register: sync_result
ignore_errors: true
- assert:
that:
- sync_result.cmd is search("'--rsh=.*'")
- sync_result.cmd is search("'--out-format=.*'")
thanks. it should reference somehow the issues with their reported problems? Can someone please have a look at those two changes: I think they would not be required if the with my change the command will be run differently: The second would be the case with my change an not the first:
The implementation might beinefficient. Fist building a string from a list, than building a list from string. |
There was a problem hiding this comment.
@flybyray Thank you for your comment. As you pointed out, my integration test example is not essential for this PR as it only tests the behavior of shlex_quote().
I also confirmed the integration test works fine now without '--allow-disabled' on my end.
Thanks again :)
Akasurde
added
the
gate
|
@gravesm @Akasurde @saito-hideki in case you didn't notice the bug report - this change has introduced a bug that breaks (formerly) correct and working synchronize tasks. Please can you take a look at the bug report #449 and suggest what to do from here. |
fix quoting for specific cmd arguments
Fixes:
ISSUE TYPE:
COMPONENT NAME:
module: synchronize